New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add SSOTeamID to Team structs #364
Conversation
@@ -49,6 +49,7 @@ type Team struct { | |||
Visibility string `jsonapi:"attr,visibility"` | |||
Permissions *TeamPermissions `jsonapi:"attr,permissions"` | |||
UserCount int `jsonapi:"attr,users-count"` | |||
SSOTeamID *string `jsonapi:"attr,sso-team-id"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose *string
instead of string
because SSOTeamID
can be nil
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good choice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good! Some minor things mentioned below...
Another thing to note is to squash the test:
commits for a cleaner commit history!
@@ -49,6 +49,7 @@ type Team struct { | |||
Visibility string `jsonapi:"attr,visibility"` | |||
Permissions *TeamPermissions `jsonapi:"attr,permissions"` | |||
UserCount int `jsonapi:"attr,users-count"` | |||
SSOTeamID *string `jsonapi:"attr,sso-team-id"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good choice!
team.go
Outdated
@@ -117,6 +121,9 @@ type TeamUpdateOptions struct { | |||
// Optional: New name for the team | |||
Name *string `jsonapi:"attr,name,omitempty"` | |||
|
|||
// Optional: Unique Identifier to control team membership via SAML | |||
SSOTeamID *string `jsonapi:"attr,sso-team-id"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the field is optional add omitempty
in the struct tag
I squashed the commits and added モ ENABLE_BETA=1 TFE_ADDRESS=https://... TF_ACC=1 go test -v ./... -tags=integration -run TestTeams
=== RUN TestTeamsList
=== RUN TestTeamsList/without_list_options
team_integration_test.go:37: paging not supported yet in API
=== RUN TestTeamsList/with_list_options
team_integration_test.go:43: paging not supported yet in API
=== RUN TestTeamsList/without_a_valid_organization
--- PASS: TestTeamsList (2.13s)
--- SKIP: TestTeamsList/without_list_options (0.25s)
--- SKIP: TestTeamsList/with_list_options (0.00s)
--- PASS: TestTeamsList/without_a_valid_organization (0.00s)
=== RUN TestTeamsCreate
=== RUN TestTeamsCreate/with_valid_options
=== RUN TestTeamsCreate/with_sso-team-id
=== RUN TestTeamsCreate/when_options_is_missing_name
=== RUN TestTeamsCreate/when_options_has_an_invalid_organization
--- PASS: TestTeamsCreate (1.48s)
--- PASS: TestTeamsCreate/with_valid_options (0.40s)
--- PASS: TestTeamsCreate/with_sso-team-id (0.23s)
--- PASS: TestTeamsCreate/when_options_is_missing_name (0.00s)
--- PASS: TestTeamsCreate/when_options_has_an_invalid_organization (0.00s)
=== RUN TestTeamsRead
=== RUN TestTeamsRead/when_the_team_exists
=== RUN TestTeamsRead/when_the_team_exists/visibility_is_returned
=== RUN TestTeamsRead/when_the_team_exists/permissions_are_properly_decoded
=== RUN TestTeamsRead/when_the_team_exists/organization_access_is_properly_decoded
=== RUN TestTeamsRead/when_the_team_exists/SSO_team_id_is_returned
=== RUN TestTeamsRead/when_the_team_does_not_exist
=== RUN TestTeamsRead/without_a_valid_team_ID
--- PASS: TestTeamsRead (1.96s)
--- PASS: TestTeamsRead/when_the_team_exists (0.18s)
--- PASS: TestTeamsRead/when_the_team_exists/visibility_is_returned (0.00s)
--- PASS: TestTeamsRead/when_the_team_exists/permissions_are_properly_decoded (0.00s)
--- PASS: TestTeamsRead/when_the_team_exists/organization_access_is_properly_decoded (0.00s)
--- PASS: TestTeamsRead/when_the_team_exists/SSO_team_id_is_returned (0.00s)
--- PASS: TestTeamsRead/when_the_team_does_not_exist (0.16s)
--- PASS: TestTeamsRead/without_a_valid_team_ID (0.00s)
=== RUN TestTeamsUpdate
=== RUN TestTeamsUpdate/with_valid_options
=== RUN TestTeamsUpdate/when_the_team_does_not_exist
=== RUN TestTeamsUpdate/without_a_valid_team_ID
--- PASS: TestTeamsUpdate (1.72s)
--- PASS: TestTeamsUpdate/with_valid_options (0.38s)
--- PASS: TestTeamsUpdate/when_the_team_does_not_exist (0.18s)
--- PASS: TestTeamsUpdate/without_a_valid_team_ID (0.00s)
=== RUN TestTeamsDelete
=== RUN TestTeamsDelete/with_valid_options
=== RUN TestTeamsDelete/without_valid_team_ID
--- PASS: TestTeamsDelete (1.31s)
--- PASS: TestTeamsDelete/with_valid_options (0.34s)
--- PASS: TestTeamsDelete/without_valid_team_ID (0.00s)
PASS
ok github.com/hashicorp/go-tfe (cached)
? github.com/hashicorp/go-tfe/examples/organizations [no test files]
? github.com/hashicorp/go-tfe/examples/workspaces [no test files]
? github.com/hashicorp/go-tfe/mocks [no test files] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not in Kansas anymore 🔥
team_integration_test.go
Outdated
@@ -321,7 +346,7 @@ func TestTeamCreateOptions_Marshal(t *testing.T) { | |||
bodyBytes, err := req.BodyBytes() | |||
require.NoError(t, err) | |||
|
|||
expectedBody := `{"data":{"type":"teams","attributes":{"name":"team name","organization-access":{"manage-policies":true},"visibility":"organization"}}} | |||
expectedBody := `{"data":{"type":"teams","attributes":{"name":"team name","organization-access":{"manage-policies":true},"sso-team-id":null,"visibility":"organization"}}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change might cause failures when run from CI because the associated feature flag is disabled by default. Should I undo this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can bring this back once we remove the feature flag and officially un-beta-fy this field!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the Disregard, you are using the helper. You could add it to this test, and to be honest I'm not sure why this test exists (since other tests already indirectly validate [un]marshaling)skipIfBeta(t)
helper for tests that require a feature sitting behind a feature flag! I think it still eludes me which feature flags are enabled for CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me! Though I will defer to a maintainer for final approval 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not from a Jedi! 🔥 Good work
Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes. |
Description
The teams API is being updated to include a new attribute called
sso-team-id
to allow SSO enabled organizations to use unique identifiers fromexternal systems to control team membership.
Output from tests (HashiCorp employees only)